Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bdt] Switch to dcargs #69

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gbalke
Copy link
Member

@gbalke gbalke commented Sep 26, 2022

Signed-off-by: Greg Balke [email protected]

Topic: bdt_uses_dcargs
Reviewers: brent

Signed-off-by: Greg Balke <[email protected]>

Topic: bdt_uses_dcargs
Reviewers: brent
@gbalke
Copy link
Member Author

gbalke commented Sep 26, 2022

Reviews in this chain:
#69 [Bdt] Switch to dcargs

@gbalke
Copy link
Member Author

gbalke commented Sep 26, 2022

# head base diff date summary
0 83f41825 f552a7fd diff Sep 25 9:13 PM 4 files changed, 94 insertions(+), 61 deletions(-)
1 0b7072fc f552a7fd diff Oct 04 12:44 AM 1 file changed, 5 insertions(+), 4 deletions(-)

@gbalke gbalke requested a review from brentyi September 26, 2022 04:13
"""


actuation: dcargs.conf.Positional[str]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably a literal could be used here?

from typing import Literal

actuation: Literal["current", "phase"], etc



actuation: dcargs.conf.Positional[str]
values: dcargs.conf.Positional[List[Tuple[float, float, float]]]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in general, variable-length positional arguments are scary because it means you can't have add more positional args afterward

@@ -35,7 +52,9 @@ def make_list(x):
def make_type(x, to_type):
return [to_type(y) for y in x]

board_ids = make_type(make_list(ast.literal_eval(args.board_ids)), int)
board_ids = make_type(
make_list(ast.literal_eval(args.motor.board_ids)), int
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this still needed? since board_ids is annotated as List[int] now

@gbalke
Copy link
Member Author

gbalke commented Oct 4, 2022

Discussed offline... I think the current behavior where betz drive tools creates a connection per tool execution is a pretty poor design for doing continuous testing. I believe dcargs will work much better if it doesn't have to deal with lists of lists of command args (which is the current hacky think I'm doing for control_motor. I'll leave this PR and hold off until I have a better architecture in place for the server system.

@gbalke gbalke force-pushed the gbalke/revup/master/bdt_uses_dcargs branch from 83f4182 to 0b7072f Compare October 4, 2022 07:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants